Skip to content

Fix stale go.work vendor warning for members (#1766) - #1767

Merged
Mikola Lysenko (mikolalysenko) merged 4 commits into
mainfrom
agent/fix-go-work-vendor-stale-member
Oct 28, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 4 commits into
mainfrom
agent/fix-go-work-vendor-stale-member

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

LLM Description written by Claude Code:claude-opus-5-5

Fixes #1766

Summary

In a go.work workspace, rollback and remove in a member other than
the root (use ./a) now warn go_vendor_modules_txt_out_of_sync, and
apply --check exits 1, when vendor/modules.txt still records the
removed socket replace. Before this change both commands exited 0 with
no warning, and every workspace build then failed with
"inconsistent vendoring".

Root cause

vendor_modules_txt_issues only judged a recorded line stale when
ReplaceEntry::socket_owned() held. detect_owner anchors that test at
the project root, by design. go work vendor records a member's
directory replace relative to the go.work directory
(./a/.socket/go-patches/…), so the line read as user-authored and was
never checked. The forward ("not recorded") check got by on a loose
path-suffix match, and that match also let a stale ./a/… line pass as
member b's ./.socket/… replace.

Fix (crates/socket-patch-core/src/vendor/go_consumer_sync.rs)

  • Recorded lines resolve against the directory that holds vendor/
    (the go.work or go.mod directory). Each go.mod/go.work replace resolves
    against its own file's directory, and the two are compared as go joins
    them (lexically). This replaces the suffix rule.
  • A recorded line also counts as socket-owned when its resolved target
    lies under any readable use member's .socket/go-patches/ or
    .socket/vendor/golang/ directory. detect_owner stays root-anchored,
    so a user's ../sibling/.socket/… replace stays user-owned.
  • A member whose go.mod can't be read is skipped. Its replaces are
    unknown, so its lines are not reported stale from another member.

No docs change: docs/ecosystems.md already documents this warning for
go work vendor. The npm, PyPI and gem wrappers don't touch this code.

Test evidence

Issue Regression test Without fix With fix
#1766 (rollback, remove; go-patches + vendored copies; nested sub/a and out-of-tree ../a members) unit vendor::go_consumer_sync::tests::workspace_member_rollback_leaves_modules_txt_stale FAILED (a .socket/go-patches/…: [], no issue reported) ok
#1766 real go 1.24.7 repro (use ./consumer, go work vendor, rollback then remove, apply --check, rebuild) e2e e2e_golang_build::go_work_vendor_member_needs_go_work_vendor_after_rollback_and_remove FAILED ("rollback must name the vendor/modules.txt regeneration") ok

The same unit test also covers the review follow-ups: a cross-member
same-spelling collision (a stale ./a/… line next to b's ./.socket/…
replace) and a member with an unreadable go.mod.

Local gates:

  • cargo fmt --all -- --check: clean
  • cargo clippy --workspace --all-features -- -D warnings: clean
  • SOCKET_PATCH_GO_E2E_REQUIRED=1 cargo test -p socket-patch-cli --all-features --test e2e_golang_build: 28 passed
  • cargo test --workspace --all-features: 15,076 passed, 12 failed. All 12 failures are environmental and outside Go code: 3 in e2e_bun_lockb fail identically on main with the local bun 1.4.4, and 9 write-failure, unremovable-lock or symlink-root tests (partial_lockfile_write_failure…, repair_cleanup_failure…, copy_tree::relax_loop…, pypi_*::wire_*failure…, vlt_heal and similar) can't simulate permission denial because this sandbox runs as root. CI runs them unprivileged.

/code-review high was run on the diff. Findings fixed: the suffix-match
false negative, the wrong-level ownership heuristic, the absolute-path
exclusion (cross-volume Windows members), false stale reports for
unreadable members, and the per-line clones and normalization. Not
fixed: the claim that the e2e comment is wrong ("rollback dropped the
manifest entry"). Rollback does remove the entry (see rollback.rs, "remove … from the local
manifest"), and the test showed re-apply discovering 0 patches until the
patch was re-staged.

https://claude.ai/code/session_011f9aeSV7eaqPG4mX6fgBKB


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
After rollback or remove in a go.work member other than the workspace
root, socket-patch stayed silent about the vendor/modules.txt line that
`go work vendor` had recorded for the patch. Every workspace build then
failed with "inconsistent vendoring" while both commands exited 0.

`go work vendor` writes a member's replace relative to the go.work
directory (./a/.socket/go-patches/...), and the stale check only treated
root-relative .socket paths as socket-owned. The audit now resolves a
recorded path against the go.work directory and treats it as ours when
it lies under any member's .socket copy directories. A user's replace
onto another checkout (../sibling/.socket/...) stays user-owned.

Adds unit tests (go-patches and vendored copies, nested and out-of-tree
members) and a real-go e2e covering rollback and remove.

Fixes #1766

Assisted-by: Claude Code:claude-opus-5-5
Review follow-up. The stale check compared a recorded workspace line
with go.mod replaces by path suffix, so a stale ./a/.socket/... line
still matched member b's ./.socket/... replace after a rollback in a
and an apply in b. Both commands then stayed silent while the build
failed "inconsistent vendoring".

Each replace is now resolved against its own file's directory and
compared with the recorded line resolved against the go.work (or
go.mod) directory, as go does. A member whose go.mod cannot be read
no longer makes its recorded lines look stale from other members.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 28, 2026 05:35
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 9376571. Configure here.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 28, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Ready for review at 9376571b (burn-down agent label sweep).

  • CI: 82/82 check runs green or skipped on this head (32 success, 50 skipped); mergeable, no conflicts.
  • Bugbot: reviewed 9376571 (check success), no review threads.
  • Look at: vendor/go_consumer_sync.rs — recorded vendor/modules.txt lines now resolve against the vendor directory and count as socket-owned under any use member's .socket/ dirs, replacing the old path-suffix match.
  • Slack announcement not sent this run (Slack send unavailable); next run retries.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does. In a go.work workspace, rollback/remove in a non-root member (use ./a) now warn that vendor/modules.txt is stale. Before, the leftover ./a/.socket/… line was treated as user-authored and the warning was missed, so the next workspace build failed with "inconsistent vendoring". The path-suffix check is replaced with a comparison of resolved paths, done the way go records them.

Risk: low. The change only reads files and only decides whether an audit warning is emitted. Outside a workspace, work_dir falls back to the project root, so behaviour there is unchanged.

Look here

  • go_consumer_sync.rs:553-594:`` each replace is resolved against its own file's dir, and each recorded line against the go.work dir.
  • go_consumer_sync.rs:601-625:``` the missing-line and stale-line loops.
  • go_consumer_sync.rs:633-645:```` member_owned. It matches whole path components, so member `a` never matches `ab/`, and `../sibling/.socket` stays user-owned.
  • go_consumer_sync.rs:866 unit tests and e2e_golang_build.rs:507, which runs real go work vendor.

Verified

  • I read the full diff. The tests would fail on main: detect_owner("./a/.socket/…") is None there, so the stale line was never checked.
  • The cross-member a/b case, an unreadable member, members outside the go.work dir (../a), and Windows separators and CRLF are all handled.
  • CHANGELOG.md is untouched and there is no leftover debug code.
  • CI is green on the head (32 passed, 50 skipped, 0 failed), and Bugbot is clean.

Changes I made: none.

Open questions (non-blocking):

  • lexical_target calls current_dir() on every comparison. That is cheap at this scale.
  • If the root member has an unreadable go.mod, it can still report a ./.socket line as stale. This existed before the PR.

Auto-merge is armed, so approving sends this straight to the merge queue.


Generated by Claude Code

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Oct 28, 2026
…dor-stale-member

# Conflicts:
#	crates/socket-patch-core/src/vendor/go_consumer_sync.rs
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor

cursor Bot commented Oct 28, 2026

Copy link
Copy Markdown

Bugbot is paused — your team has an unpaid invoice

Bugbot is paused because your team has an unpaid invoice.

Pay it from the billing page in the Cursor dashboard and Bugbot resumes automatically.

(requestId: 1a13b573-aab2-4fb1-9eab-55a41a9bbe20)

Merged via the queue into main with commit 4579007 Oct 28, 2026
49 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the agent/fix-go-work-vendor-stale-member branch October 28, 2026 14:56
Mikola Lysenko (mikolalysenko) pushed a commit that referenced this pull request Oct 28, 2026
Main landed lighter fixes for the same go.work gaps this PR models in
full: #1752 (version gate reads every member), #1753 (hosted pins
outranked by another member), #1767 (stale go.work vendor warning) and
the squashed #1748. Resolve them onto this PR's GoWorkspace model
instead of keeping two workspace readers:

- go_build_list: keep project_selection on GoWorkspace. Drop main's
  ModuleEvidence / workspace_selection / other_workspace_members and
  its three on-disk selection tests: GoWorkspace::selection covers
  those cases from the module graph (go_workspace tests), and without
  a module cache it answers Unknown by design.
- hosted: drop #1753's go_members plumbing (engine RewriteOptions
  closure, rewrite_golang argument, CLI wiring). The PR's
  HostedGoWorkspace already refuses a pin another member outranks.
  Main's e2e test for it now seeds the module cache the workspace
  graph reads, and passes, including the rescan that removes the pin.
- go_consumer_sync: port #1767's modules.txt audit (per-member replace
  resolution, member_owned, unreadable members skipped) onto
  go_workspace::load and its shared same_dir_key / lexical_target.
- vex discover, CLI_CONTRACT, e2e_golang_hosted_build: keep the PR's
  stricter rules (a zip pin is checked against the workspace selection
  too; sum-file zip lines are no evidence of it).

Co-Authored-By: Claude <noreply@anthropic.com>
Mikola Lysenko (mikolalysenko) pushed a commit that referenced this pull request Oct 28, 2026
Brings in the base's main merge (678f90e). It replaced
go_consumer_sync's same_replacement with lexical_target comparison
(main #1767), so the go.work modules.txt check here now compares the
go.work replace and the recorded line the same way, both resolved
against the go.work directory.

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

4 participants